feat(GrpcServlet): ability override method name#11825
feat(GrpcServlet): ability override method name#11825long76 wants to merge 24 commits intogrpc:masterfrom
Conversation
|
don't understand why crash tests) error in test env? |
found error - |
kannanjgithub
left a comment
There was a problem hiding this comment.
Left a minor comment.
…variable initialization
kannanjgithub
left a comment
There was a problem hiding this comment.
Some more changes.
|
@ejona86 ping |
|
@panchenko, do you have any opinions for this PR? |
|
@ejona86 The use case feels legit when trying to use gRPC in big legacy web applications. Another way to achieve that would be making diff --git a/servlet/src/main/java/io/grpc/servlet/GrpcServlet.java b/servlet/src/main/java/io/grpc/servlet/GrpcServlet.java
index f68ed0835..e2adba4c5 100644
--- a/servlet/src/main/java/io/grpc/servlet/GrpcServlet.java
+++ b/servlet/src/main/java/io/grpc/servlet/GrpcServlet.java
@@ -39,7 +39,10 @@ public class GrpcServlet extends HttpServlet {
private final ServletAdapter servletAdapter;
- GrpcServlet(ServletAdapter servletAdapter) {
+ /**
+ * Constructor for use by subclasses.
+ */
+ protected GrpcServlet(ServletAdapter servletAdapter) {
this.servletAdapter = servletAdapter;
}
diff --git a/servlet/src/main/java/io/grpc/servlet/ServletAdapter.java b/servlet/src/main/java/io/grpc/servlet/ServletAdapter.java
index 4bfe89497..cf5eb83ca 100644
--- a/servlet/src/main/java/io/grpc/servlet/ServletAdapter.java
+++ b/servlet/src/main/java/io/grpc/servlet/ServletAdapter.java
@@ -45,6 +45,7 @@ import java.util.Arrays;
import java.util.Enumeration;
import java.util.List;
import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
import java.util.logging.Logger;
import javax.servlet.AsyncContext;
import javax.servlet.AsyncEvent;
@@ -75,15 +76,18 @@ public final class ServletAdapter {
private final ServerTransportListener transportListener;
private final List<? extends ServerStreamTracer.Factory> streamTracerFactories;
+ private final Function<HttpServletRequest, String> methodNameResolver;
private final int maxInboundMessageSize;
private final Attributes attributes;
ServletAdapter(
ServerTransportListener transportListener,
List<? extends ServerStreamTracer.Factory> streamTracerFactories,
+ Function<HttpServletRequest, String> methodNameResolver,
int maxInboundMessageSize) {
this.transportListener = transportListener;
this.streamTracerFactories = streamTracerFactories;
+ this.methodNameResolver = methodNameResolver;
this.maxInboundMessageSize = maxInboundMessageSize;
attributes = transportListener.transportReady(Attributes.EMPTY);
}
@@ -119,7 +123,7 @@ public final class ServletAdapter {
AsyncContext asyncCtx = req.startAsync(req, resp);
- String method = req.getRequestURI().substring(1); // remove the leading "/"
+ String method = methodNameResolver.apply(req);
Metadata headers = getHeaders(req);
if (logger.isLoggable(FINEST)) {
diff --git a/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java b/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java
index 72c4383d2..5c9af0bcf 100644
--- a/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java
+++ b/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java
@@ -49,8 +49,10 @@ import java.net.SocketAddress;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.ScheduledExecutorService;
+import java.util.function.Function;
import javax.annotation.Nullable;
import javax.annotation.concurrent.NotThreadSafe;
+import javax.servlet.http.HttpServletRequest;
/**
* Builder to build a gRPC server that can run as a servlet. This is for advanced custom settings.
@@ -64,6 +66,8 @@ import javax.annotation.concurrent.NotThreadSafe;
@NotThreadSafe
public final class ServletServerBuilder extends ForwardingServerBuilder<ServletServerBuilder> {
List<? extends ServerStreamTracer.Factory> streamTracerFactories;
+ private Function<HttpServletRequest, String> methodNameResolver =
+ req -> req.getRequestURI().substring(1); // remove the leading "/"
int maxInboundMessageSize = DEFAULT_MAX_MESSAGE_SIZE;
private final ServerImplBuilder serverImplBuilder;
@@ -98,7 +102,7 @@ public final class ServletServerBuilder extends ForwardingServerBuilder<ServletS
* Creates a {@link ServletAdapter}.
*/
public ServletAdapter buildServletAdapter() {
- return new ServletAdapter(buildAndStart(), streamTracerFactories, maxInboundMessageSize);
+ return new ServletAdapter(buildAndStart(), streamTracerFactories, methodNameResolver, maxInboundMessageSize);
}
/**
@@ -176,6 +180,15 @@ public final class ServletServerBuilder extends ForwardingServerBuilder<ServletS
throw new UnsupportedOperationException("TLS should be configured by the servlet container");
}
+ /**
+ * Specifies how to determine gRPC method name from servlet request.
+ * The default strategy is using {@link HttpServletRequest#getRequestURI()} without the leading slash.
+ */
+ public ServletServerBuilder methodNameResolver(Function<HttpServletRequest, String> methodResolver) {
+ this.methodNameResolver = checkNotNull(methodResolver);
+ return this;
+ }
+
@Override
public ServletServerBuilder maxInboundMessageSize(int bytes) {
checkArgument(bytes >= 0, "bytes must be >= 0");And then in application code public class MyServlet extends GrpcServlet {
public MyServlet() {
super(new ServletServerBuilder()
.addService(new MyService())
.methodNameResolver(req -> req.getRequestURI().substring(req.getContextPath().length() + 1))
.buildServletAdapter());@long76 would that work for you? |
|
@panchenko, yeah, I agree the use case is legitimate. But I'm also wary without reminding myself the difference between all the different servlet path-related strings (context, servlet name, path info, path translated, etc). I mainly just remember there were many ways of doing this sort of thing wrong, and it had seemed the "proper" way was rarely known online (at the time, decade ago). |
|
FWIW, I do think I'd prefer simple configuration for common use cases. This seems common enough that it doesn't seem like it should deserve an lambda to configure. As an old-school servlet user, this seems more like something that would have been nicest to configure in web.xml. |
|
@panchenko i think about this way(more universal), need to check. I decide remove only context path to expect user mistake and match grpc default functional - how i know non servlet usage ways don't allow user override method name out-of-the-box. Your solution may make servlet more prefer as more flexible tool but servlet still experimental. |
|
The lambda approach is more flexible — for example, if someone wants to strip a different prefix rather than contextPath. Using a servlet parameter doesn’t really fit here (whether in web.xml or annotations), since For servlet-based apps, the cleanest way lifecycle-wise would be:
|
|
@panchenko sorry for long answer, your solution works, feel free to create MR. i provide some other solution here |
|
@long76 thanks for confirming, and just to reiterate, have you considered approaches which don't require changes in the library code?
|
|
|
so, feel free to close this MR because you will provide yours or approve and merge this) |
|
@long76 In the first approach I mean do not change request path in nginx, but rename the context to match your gRPC service. For example, for |
i see, it's should work, but we have many services and add for all uncomfortable) |
|
#12333 was merged, close this |
Refs: #5066
Complex application may have many
war-s and do not allow monopolize root context path(/). Add ability to override method name just overridegetMethodfromGrpcServletUsage example, for remove context path:
Links:
https://stackoverflow.com/a/42706412
https://coderanch.com/t/610432/certification/getContextPath-getServletPath-getPathInfo